-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: store artifacts in OCI registry #3328
Conversation
0175633
to
c4ff2b2
Compare
69b2ad2
to
81a3df3
Compare
cf14e66
to
5e29180
Compare
@@ -280,6 +280,8 @@ jobs: | |||
run: just pnpm-install | |||
- name: Build Language Plugins | |||
run: just build-language-plugins | |||
- name: Pull Registry | |||
run: docker image pull registry:2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just use docker compose
for this yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason it was failing to pull on CI, and I have no idea why. Explicitly pulling it worked for some reason. I can remove this and try again in case it was a transient failure, but it was very odd.
// in the interim releases and artefacts will continue to be linked via the `deployment_artefacts` table | ||
Handle *libdal.Handle[ContainerService] | ||
db sql.Querier | ||
type OCIArtifactService struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been using "artEfact" everywhere else
backend/runner/runner.go
Outdated
HeartbeatJitter time.Duration `help:"Jitter to add to heartbeat period." default:"2s"` | ||
Deployment string `help:"The deployment this runner is for." env:"FTL_DEPLOYMENT"` | ||
DebugPort int `help:"The port to use for debugging." env:"FTL_DEBUG_PORT"` | ||
Registry artefacts.RegistryConfig `embed:""` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have prefix:"oci-"
otherwise the flags will be difficult to distinguish from other ones in the future.
@@ -41,3 +41,7 @@ provisioner: | |||
istio: | |||
enabled: true | |||
|
|||
registry: | |||
repository: "ftl-registry:5000/ftl-artefacts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly not for now, but should we be namespacing these by realm at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the repository used in the GH Kube tests.
frontend/cli/cmd_box_run.go
Outdated
RunnerBase *url.URL `help:"Base bind address for FTL runners." default:"http://127.0.0.1:8893" env:"FTL_RUNNER_BIND"` | ||
Dir string `arg:"" help:"Directory to scan for precompiled modules." default:"."` | ||
ControllerTimeout time.Duration `help:"Timeout for Controller start." default:"30s"` | ||
Registry artefacts.RegistryConfig `embed:""` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registry artefacts.RegistryConfig `embed:""` | |
Registry artefacts.RegistryConfig `embed:"" prefix:"oci-"` |
frontend/cli/cmd_download.go
Outdated
Deployment model.DeploymentKey `help:"Deployment to download." arg:""` | ||
Dest string `short:"d" help:"Destination directory." default:"."` | ||
Deployment model.DeploymentKey `help:"Deployment to download." arg:""` | ||
Registry artefacts.RegistryConfig `embed:""` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a prefix.
I feel like we should be injecting the registry instance itself rather than the config? ie. put the config in the root CLI struct, and add a bind function for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and release will be the only commands that directly interact with the registry though, in most other cases we would construct it but not use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't construct it unless it's injected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even so won't it 'pollute' all the other commands with OCI flags that do nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that regard it's similar to the controller endpoint, which is unused by some commands but used in enough places that it warrants being global. This will be used in at least 5 locations that I'm aware of - download, serve/dev, release, deploy, box
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serve/dev don't use it, as they start their own registry, deploy uploads through the controller so that the local user does not need OCI credentials. Thinking about it I am not sure if we want download / release to actually interact directly with the registry long term either, rather than going through a controller so the controller can gate the interactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of agree, in which case we should keep the current gRPC interface...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And continue to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sorry, for downloads - WDYT?)
internal/download/download.go
Outdated
) | ||
|
||
// Artefacts downloads artefacts for a deployment from the Controller. | ||
func Artefacts(ctx context.Context, client ftlv1connect.ControllerServiceClient, key model.DeploymentKey, dest string) error { | ||
func Artefacts(ctx context.Context, client ftlv1connect.ControllerServiceClient, key model.DeploymentKey, dest string, registry artefacts.RegistryConfig) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be cleaner if we injected the artefact client rather than the config, I think. Better for testing etc.
fae4665
to
fe75727
Compare
67dd5ac
to
2d7a3e7
Compare
06c6e04
to
e4711a6
Compare
l have changed this so download now goes through the controller. |
This removes the artefacts table and instead stores all binaries in a container registry. This means that FTL now requires a registry to operate, and in dev mode it is spun up automatically in a similar manner to postgres. Uploads are done via the controller, and the runner pulls content directly from the registry.
e4711a6
to
e0e6a0e
Compare
Initial draft of storing artifacts in the OCI registry, not really ready for review and needs a lot of work:
Basically: